feat(add-xy_shape): Ability to use xy_shape field type #885

kmessaoudi · 2024-03-07T10:03:42Z

Description

Currently when using the shape property with the opensearch java client, this result as an error as it is known to OpenSearch, where this feature has the name xy_shape.
We were unsure of shape usage and didn't know if we should have removed it.
This PR adds the ability of using Xy shape property.

Issues Resolved

Fixes #884

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

CHANGELOG.md

java-client/src/main/java/org/opensearch/client/opensearch/_types/mapping/FieldType.java

java-client/src/main/java/org/opensearch/client/opensearch/_types/mapping/XyShapeProperty.java

java-client/src/main/java/org/opensearch/client/opensearch/_types/query_dsl/XyShapeQuery.java

...lient/src/main/java/org/opensearch/client/opensearch/_types/query_dsl/XyShapeFieldQuery.java

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

java-client/src/test/java/org/opensearch/client/opensearch/model/BehaviorsTest.java

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

reta · 2024-03-18T15:19:58Z

@dblock any concerns from your side? thank you!

dblock

I see your conversation around license. The XyShapeQuery code looks suspiciously like the existing ShapeQuery code, so I am afraid we need to put back the Elastic license header in there. Even if they refer to two different concepts, I think we should be safe vs. sorry.

kmessaoudi · 2024-03-19T07:49:18Z

Hi @dblock
Thank you for your answer
It is the same code, yes.
Is it that header you want me to put back ?

/*
 * Licensed to Elasticsearch B.V. under one or more contributor
 * license agreements. See the NOTICE file distributed with
 * this work for additional information regarding copyright
 * ownership. Elasticsearch B.V. licenses this file to you under
 * the Apache License, Version 2.0 (the "License"); you may
 * not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing,
 * software distributed under the License is distributed on an
 * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
 * KIND, either express or implied.  See the License for the
 * specific language governing permissions and limitations
 * under the License.
 */

/*
 * Modifications Copyright OpenSearch Contributors. See
 * GitHub history for details.
 */

reta · 2024-03-19T11:50:14Z

Hi @dblock Thank you for your answer It is the same code, yes. Is it that header you want me to put back ?

/*
 * Licensed to Elasticsearch B.V. under one or more contributor
 * license agreements. See the NOTICE file distributed with
 * this work for additional information regarding copyright
 * ownership. Elasticsearch B.V. licenses this file to you under
 * the Apache License, Version 2.0 (the "License"); you may
 * not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing,
 * software distributed under the License is distributed on an
 * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
 * KIND, either express or implied.  See the License for the
 * specific language governing permissions and limitations
 * under the License.
 */

/*
 * Modifications Copyright OpenSearch Contributors. See
 * GitHub history for details.
 */

@kmessaoudi I think what @dblock means is that: for new files - the header is correct (SPDX one), but for modified files (which also included renamed like Shape -> XyShape) would be more correct to preserve the original license header.

dblock · 2024-03-19T12:36:10Z

Yes, please. Sorry for being annoying and appreciate your work!

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

dblock · 2024-03-19T13:54:41Z

...t/src/test/java/org/opensearch/client/opensearch/_types/query_dsl/XyShapeFieldQueryTest.java

@@ -0,0 +1,14 @@
+package org.opensearch.client.opensearch._types.query_dsl;


This one needs a license header too (only SPDX for the new file, but this one does look like a copy of the old ShapeFieldQueryTest. CI should be catching the missing one - if it's not and it's not a trivial fix, open a GH issue please?

Ok I've checked the other files in test directory before pushing, they don't have license header ?
It is indeed a copy of ShapeFieldQueryTest, the header wasn't there before, is that normal ?
I am not sure to understand

Sorry if I wasn't clear.

It should have been there and CI should have caught this as a problem before.

Please do add it, including the ES one since it's an old file.

We need to make sure license checker catches this in CI, if the problem is not obvious (excluded folder or something), please open an issue?

It is indeed a copy of ShapeFieldQueryTest, the header wasn't there before, is that normal ?
I am not sure to understand

@kmessaoudi oh this seems like a miss on our side, thank you for pointing it out, I believe we need to have license headers there, @VachaShah may be it rings a bell to you?

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

opensearch-trigger-bot · 2024-03-19T15:09:16Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-885-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 ccdb56a31dca1184b29b3948ee66835df5681dd0
# Push it to GitHub
git push --set-upstream origin backport/backport-885-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-885-to-2.x.

dblock · 2024-03-19T15:09:45Z

@kmessaoudi I merged the PR. Looks like automatic backport failed, want to do a manual one to the 2.x branch so we can have this in a 2.x release?

…oject#885) * feat(add-xy_shape): Ability to use xy_shape field type Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com> * update changelog Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com> * Fix changelog and add license header Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com> * updated license header Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com> * remove shape property Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com> * fix test after removing shape Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com> * adapt ShapeQuery test to XyShapeQuery Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com> * Add ES license headers Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com> * Add ES license headers on tests Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com> --------- Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com> (cherry picked from commit ccdb56a)

* feat(add-xy_shape): Ability to use xy_shape field type Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com> * update changelog Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com> * Fix changelog and add license header Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com> * updated license header Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com> * remove shape property Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com> * fix test after removing shape Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com> * adapt ShapeQuery test to XyShapeQuery Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com> * Add ES license headers Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com> * Add ES license headers on tests Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com> --------- Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com> (cherry picked from commit ccdb56a)

kmessaoudi requested review from reta, Bukhtawar, dblock, szczepanczykd, madhusudhankonda, saratvemulapalli and VachaShah as code owners March 7, 2024 10:03

kmessaoudi force-pushed the feat/add-xy_shape-fields branch 2 times, most recently from 1bf9197 to 4ace680 Compare March 7, 2024 10:10

feat(add-xy_shape): Ability to use xy_shape field type

d4a64ac

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

kmessaoudi force-pushed the feat/add-xy_shape-fields branch from 4ace680 to d4a64ac Compare March 7, 2024 10:18

update changelog

3505e64

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

kmessaoudi force-pushed the feat/add-xy_shape-fields branch from b4630b7 to 3505e64 Compare March 15, 2024 13:38